Skip to content

Conversation

TimWolla
Copy link
Member

@TimWolla TimWolla commented Oct 8, 2025

The space after & looked super odd. Remove it and put the & right before the function name (after the class name), matching the actual syntax for declaring a by-ref return.

…function_declaration()

The space after `&` looked super odd. Remove it and put the `&` right before
the function name (after the class name), matching the actual syntax for
declaring a by-ref return.
Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed previously, I'm in favor of the idea.
I think hooks may need a tweak though.

?>
--EXPECTF--
Fatal error: Declaration of A::$prop::get() must be compatible with & I::$prop::get() in %s on line %d
Fatal error: Declaration of A::$prop::get() must be compatible with I::&$prop::get() in %s on line %d
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one looks weird, as the signature of reference get hook is &get, not &$prop.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I had also seen that. This isn't quite trivial to fix, since it the technical method name is $prop::get as a single string. Moving the & in front of the class name (just removing the space) would work. My main issue is with the space.

/cc @Crell @iluuu1994

Copy link
Member

@iluuu1994 iluuu1994 Oct 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what I think of this change, but I::$prop::&get() seems like the consistent option. I don't think I::&$prop::get() is better than what we currently have. But I also acknowledge this is likely not important enough to warrant a complex fix.

I think overall I'd prefer not to adjust the message at all (apart from maybe removing the space).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants